re-callable trackedFunction
#559
Conversation
|
I haven't tested this fully with tracking context yet but its a work in progress and i figured i'd open a draft pr. |
|
I know it's not necessarily nice to put values on a function object, but i think the ergonomics of this approach are quite nice. I also played around a bit with putting a "re-execute" function onto the |
velrest
force-pushed
the
retry-trackedfunction
branch
3 times, most recently
from
066fb9a to
ad279cf
Compare
ember-resources/src/util/function.ts
Outdated
| return propertyValue.state; | ||
| }; | ||
|
|
||
| Object.defineProperty(propertyValue, "value", { |
There was a problem hiding this comment.
This likely won't be TS safe -- any reason to not use a class?
There was a problem hiding this comment.
I'm not sure how you can define a class which can then be called as a function. Just out of interest, do you know how to do this?
There was a problem hiding this comment.
why call the class as a function? it can have a method that can be called
I think we'll still want to explore putting things on the |
Out of curiosity, is it because of the API or maybe because the implementation is a bit messy? |
classes have way better type inference than the dynamic approach presently implemented in this PR. ember-resources is aiming for maximum Glint support with maximum type inference ability (yay!) 🥳 |
|
I see, that`s a very good argument 😄 . I'm not really a typescript user and am more used to a more "functional" programming style so that's why i tried to implement it a bit more declarative and in a dynamic way. I'll have a look again and put this into the |
velrest
force-pushed
the
retry-trackedfunction
branch
5 times, most recently
from
0eaaca5 to
c867dc0
Compare
| @@ -1,15 +1,22 @@ | |||
| import { tracked } from '@glimmer/tracking'; | |||
There was a problem hiding this comment.
why so many formatting changes here? can you revert these? makes the diff a bit noisy
There was a problem hiding this comment.
Done, prettier format on save 😓
velrest
force-pushed
the
retry-trackedfunction
branch
from
c867dc0 to
8421b63
Compare
ember-resources/src/util/function.ts
Outdated
|
|
||
| (async () => { | ||
| return resource<TrackedFunctionProperty<Return>>(context, (hooks) => { | ||
| const getValue = async (state: State<Return>) => { |
There was a problem hiding this comment.
Can this function move inside the new class?
velrest
force-pushed
the
retry-trackedfunction
branch
4 times, most recently
from
e73f949 to
6cbc23f
Compare
|
@NullVoxPopuli I implemented the function on the class as you proposed an the test is now green, feel free to have a look at this again 😄 |
velrest
force-pushed
the
retry-trackedfunction
branch
from
6cbc23f to
2f5d0d5
Compare
| return resource<State<Return>>(context, (hooks) => { | ||
| let state = new State(initialValue); | ||
| return resource<TrackedFunctionProperty<Return>>(context, (hooks) => { | ||
| return new TrackedFunctionProperty(fn, hooks, initialValue); |
There was a problem hiding this comment.
Changing the return type is a bit of a breaking change -- any chance you can combine the two classes, in in the execute method, "reset" the state values?
| { id: 2, page: 2 }, | ||
| ]); | ||
|
|
||
| const value = await foo.data.execute(); |
There was a problem hiding this comment.
can you add assertions for what the value properties are supposed to be after execute is invoked?
maybe something like:
let almostValue = foo.data.execute();
// assert stuff
assert...
let value = await almostValue|
|
||
| constructor(public initialValue?: Value) {} | ||
| @action | ||
| execute() { |
There was a problem hiding this comment.
why "execute" for the name?
There was a problem hiding this comment.
I'm also not happy with it but perform is no kind of reserved for tasks, run, call etc also seem a bit off and retry is imo trying something that has failed again. Whats your opinion?
|
I gave a shot at this here: #665 thoughts? |
No description provided.